Skip to content

Implement Oplog epoch fencing: oplog_metadata, epoch-checked appends, ShardLost relinquish - #3845

Open
Aditya1404Sal wants to merge 10 commits into
golemcloud:mainfrom
Aditya1404Sal:shard-manager/ticket5-oplog-epoch-fence
Open

Aditya1404Sal wants to merge 10 commits into
golemcloud:mainfrom
Aditya1404Sal:shard-manager/ticket5-oplog-epoch-fence

Conversation

@Aditya1404Sal

@Aditya1404Sal Aditya1404Sal commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Resolves GOL-449

Implement Oplog epoch fencing: oplog_metadata, epoch-checked appends, ShardLost relinquish

Ticket 4 made shard ownership a lease. A lease bounds how long a lost shard keeps being served, but it
cannot stop an executor that has already lost the shard from finishing a write it had started. An
executor frozen by a pause, a VM migration or a partition wakes up still believing it owns its agents.
Until now the only thing standing between its writes and the new owner's was the index_storage
primary key, which kills whichever executor happens to write second — often the rightful owner.

This PR records, per oplog, the shard epoch allowed to write it, and checks every batch of entries
against that record inside the transaction that inserts them. An executor whose epoch is behind is
refused at the storage. The one agent it was writing is given up (ShardLost): nothing more is written
for it, and the worker service resumes it on the shard's owner. The executor keeps serving everything
else.

Builds on #3834. #3766 (revoke drain) lands after this and builds on ActiveAgents::relinquish_matching.

What this adds

The fence in storage

  • An ownership record per oplog. oplog_metadata (namespace, key, epoch) in the PostgreSQL and
    SQLite indexed storage (002_oplog_metadata.sql). It is written before an oplog's first entry,
    removed before its entries, and only moves forward: the upsert is a compare-and-set
    (WHERE oplog_metadata.epoch <= EXCLUDED.epoch).
  • Every append asserts the epoch in the transaction that inserts it. PostgreSQL reads the record
    with SELECT ... FOR UPDATE; SQLite relies on its single-connection write pool (the code says what
    raising that cap would require). A missing record refuses too, and a refused batch leaves nothing
    behind. SQLite and multi-SQLite get their own transactional append_many.
  • The epoch travels on the storage abstraction. IndexedStorage::append/append_many take
    shard_epoch: Option<ShardEpoch>. None asserts nothing: single-shard mode, the debugging service,
    a fork into a remote target, ephemeral oplogs and the archive layers. Backends that cannot fence say
    so through supports_epoch_fencing.

A refused write gives up one agent

  • An oplog asserts the epoch its executor held when it opened it, claimed before the last index is
    read, so an executor that already lost the shard is refused before it replays.

  • The refusal latches. IndexedStorageError::FencedOplogError::Fenced
    WorkerExecutorError::OplogFencedTrapType::Interrupt(InterruptKind::ShardLost). After the first
    refusal every add, pair, start and commit on that handle is refused without asking the storage.
    Entries buffered before the refusal stay readable, so a reader that took the index earlier never
    finds a gap.

  • Refused writes are reported, never acknowledged. Oplog and the Worker commit APIs
    (commit_oplog_and_update_state, add_and_commit_oplog) return the fence. Callers propagate it, so
    nothing is done for an entry the storage refused:

    • AgentInvocationFinished is committed before a result is published to waiters;
    • an invocation is accepted only after its PendingAgentInvocation committed (unary and streaming);
    • a remote transaction's pre-commit/pre-rollback marker, a custom durable Start and the replay
      Jumps gate the side effect they precede;
    • cancel, revert, update and plugin (de)activation are not acknowledged;
    • an in-function retry whose Error entry is refused traps instead of retrying the effect.

    Transient storage failures keep panicking exactly as before; only the fence changes behaviour.

  • Given up, never restarted in place. The agent is stopped without writing to its oplog or status
    and removed from ActiveAgents (scoped to its generation). OplogFenced crosses the wire as
    ShardingNotReady, which the worker service answers by refreshing its routing table and retrying on
    the owner. Pending invocations of a given-up agent get that error and no cached result, because
    the new owner runs them.

  • Revoked and narrowed shards give their agents up instead of restarting them with
    InterruptKind::Restart, which would reopen the oplog at a stale epoch.

  • The invocation loop never waits for a stop that waits for it. Once an agent is given up the loop
    stops picking up work, never creates a new instance, and writes no lifecycle, failure, update or
    streaming-session record for it. Loop-side cancel and manual-update enqueue refuse instead of waiting
    when the worker is stopping.

  • A sweep never stalls lease renewal. Sweeps triggered by the renewal loop run on a single-flight
    task, so a slow relinquish (for example of an agent still loading) cannot let the lease lapse.
    AssignShards and startup still await the sweep.

Keeping epochs meaningful

  • The shard manager repairs lost history automatically. A renewal claim above the recorded epoch
    raises the manager's floor. A re-registration after LeaseNotFound carries the shards the executor
    held (RegisterRequest.previous_shard_epochs). Executors report the epochs their refused writes saw
    (RenewShardLeaseRequest.fenced_shard_epochs), and the manager mints above them. Out-of-range epochs
    from the wire are ignored rather than overflowing.
  • AgentInvocationStarted records the epoch that wrote it (shard_epoch: Option<u64>, raw oplog
    only; forks clear it).
  • A routing miss before acceptance is retried, not surfaced. The new
    INVOCATION_REJECTION_REASON_SHARDING_NOT_READY carries InvalidShardId, ShardingNotReady and
    OplogFenced; the worker service retries it on the unary and streaming paths, and agent RPC maps it
    the same way. Before, InvalidShardId fell through to Internal, a terminal error.
  • Startup refuses storage that cannot fence under a real shard manager
    (ShardManagerService::requires_oplog_fencing has no default on purpose).
  • Fences and give-ups are logged with the agent and both epochs.

Supporting changes

  • The test framework can pause and resume a spawned executor (SIGSTOP/SIGCONT): it looks dead to the
    cluster while it still believes it owns its shards, the case this PR is about.
  • Docs: the deployment guide describes the fence, the backends that enforce it, the automatic repair
    and the rollout order; the persistence page lists PostgreSQL and SQLite as indexed storage.
    worker-executor-walkthrough.html and the understanding-durable-execution skill describe ShardLost,
    the fence and fallible commits.
  • local-run/start.sh runs the executor and the debugging service on SQLite indexed storage.

Operational change and rollout

  • An executor configured with Redis or in-memory indexed storage and a shard manager refuses to
    start, naming the setting to change (GOLEM__INDEXED_STORAGE__TYPE: Postgres, Sqlite,
    KVStoreSqlite, MultiSqlite or KVStoreMultiSqlite). The shipped default is still KVStoreRedis,
    so a distributed deployment on defaults fails at boot; changing the default belongs to ticket 6.
  • Upgrade the shard manager and worker services before the executors. An upgraded executor reports
    a routing miss with the new rejection reason, which only an upgraded worker service retries, and sends
    epoch evidence in fields an older shard manager ignores.
  • Roll forward only once any executor runs this release: migration 002 adds the table every
    append checks.
The ticket's plan, and how each part landed

The ticket had two halves: the executor side of the lease protocol, and oplog epoch fencing.

Lease protocol — landed in #3834 (ticket 4), not reopened here.

Ticket Landed as
A fresh ExecutorId per process, passed to every shard manager call #3834; re-registration after LeaseNotFound takes a fresh id
ShardAssignment carries shard_epochs and expiry; epoch_for_worker #3834 (ShardAssignment::epoch_of); this PR reads it when opening an oplog
renew_shard_lease / update_lease, background renewal loop at a third of the TTL #3834; this PR moves renewal-driven sweeps off that loop
StaleEpoch or LeaseNotFound → re-register #3834 removed StaleEpoch: a mismatched claim is answered with the corrected set
Transient failure: keep serving, retry #3834
deregister on graceful shutdown #3834 (tracked deregister on SIGTERM)

Oplog epoch fencing — this PR.

Ticket This PR Why it differs
oplog_metadata (namespace, key, epoch) table As specified
upsert_oplog_metadata overwrites a different epoch Compare-and-set, only moves forward A frozen executor also "verified it holds the lease"; an overwrite lets it un-fence itself
delete_oplog_metadata on worker deletion As specified, record removed before entries A writer still holding a deleted oplog is refused
append_many signature unchanged; Postgres-only override takes the epoch shard_epoch on the IndexedStorage trait Storage is reached through Arc<dyn IndexedStorage>; an inherent override is unreachable
SELECT ... FOR UPDATE inside the insert transaction As specified on PostgreSQL; SQLite relies on its single-connection write pool
RepoError::OplogFenced A private FencedTxError carrier instead with_tx_err needs E: From<RepoError>, which IndexedStorageError deliberately lacks
IndexedStorageError::Fenced, not retried As specified, typed as ShardEpoch
retry_storage_op returns the fence Upstream's retry_oplog_append (which reconciles indeterminate writes) made fenceable Dropping its reconciliation would have been a silent regression
Interrupt the worker with Restart InterruptKind::ShardLost: give up, never restart A restart reopens the oplog at the same stale epoch, refused forever under a partition
create/open upsert before use, epoch from the worker's shard As specified, claimed before the last index is read Otherwise the loser can append between the read and the claim
SQLite "same treatment" Own transactional append_many for SQLite and multi-SQLite Neither overrode it; the per-entry default could leave a prefix behind
Redis unfenced, acceptable for development Refuses to start with a real shard manager Redis is the shipped default, so "development only" meant every default deployment
Single-entry append wrapped in a transaction Delegates to append_many; an unasserted single entry stays one autocommit insert One fenced path; unfenced writers pay no transaction
AgentInvocationStarted.shard_epoch: u64 (raw and public), 0 for old entries Option<u64>, raw only 0 is a real epoch; the public oplog cannot see it anyway

Beyond the ticket: the fallible Oplog refactor that makes a clean per-agent stop possible, the
revoke/narrowing give-up, the startup guard, automatic epoch repair in the shard manager, the routing-miss
rejection reason, and the pause/resume test support with its end-to-end test.

Decisions made during the work

Design (before implementation)

  • Clean stop everywhere. A fence stops that one agent; it does not abort the executor (the ticket's
    panic-on-Other stays only for real storage failures). This required the fallible Oplog refactor.
  • Drain in scope, through the same primitive. Revoke and narrowing give agents up rather than
    restarting them. "Clean stop" plus "drain out of scope" would have built the stop and only fired it
    after a wasted fenced write.
  • Refuse to start on Redis or in-memory indexed storage with a real shard manager, derived from
    existing config; no new setting.
  • Monotonic compare-and-set; epoch on the trait; epoch cached at open; missing record = fenced;
    shard_epoch: Option<u64>.
  • A fork into a remote target opens unfenced (None). The default indexed storage stays KVStoreRedis
    for now. No drain on local lease expiry — only definitive signals from the shard manager. Graceful
    shutdown stays ticket 4's.
  • Ephemeral agents and archive layers are not fenced.
  • Error rule: a caller whose signature can carry an error propagates the fence; one that cannot
    handles Fenced locally (relinquish or stop) and keeps the panic for OplogError::Storage.

First review round

  • Changing the shipped default config is deferred to ticket 6.
  • RegisterRequest.previous_shard_epochs and the renewal floor raise land here. The floor raise
    deliberately reverses two ticket-4 test assertions: a claim above the record now re-mints above it.
  • Epoch regression is repaired automatically from executor evidence, not by an operator.
  • Rollout order and roll-forward-only are documented rather than coded around.
  • The suite-wide 30 s test lease override was dropped; the paused-executor e2e pauses for 90 s instead.
  • An unasserted single-entry append skips the transaction.
  • Status-flusher gating and epoch checks on drop_prefix belong to Fix permanent shard loss when RevokeShards times out #3766.

Final review round

  • A given-up agent's pending invocations get a retriable error and no cached result.
  • Renewal-driven sweeps run off the renewal loop; relinquish_matching and "relinquish awaits the loop's
    exit" are unchanged for Fix permanent shard loss when RevokeShards times out #3766.
  • After a fence, refused entries stay readable in memory rather than rolling the oplog index back.
  • The quota-lease overflow fix found by the audit stays in this PR.
Review findings fixed

First review round — 41 verified findings: 39 fixed, 1 deferred (shipped default config → ticket 6),
1 not a defect (an old worker service rejects reason 15 as a protocol error, not a type-checker error).

  • Every single-entry Postgres append cost a transaction, even unasserted.
  • Deleting the record drops the epoch high-water mark (documented, with a test).
  • Fence rejections were logged as failed repo transactions.
  • No storage test pinned that a record below the asserted epoch fences.
  • Repair after a wiped or replaced shard-manager store could never fire.
  • Backup restore: a higher claim from a non-owner was dropped.
  • Docs described a single-shard exemption operators cannot configure.
  • A cache hit ignored the requested epoch, so an older unfenced handle kept the record behind.
  • The plugin forwarding actor kept retrying checkpoints after a fence.
  • A no-op pending_uploads.clear() on the fence path; a misplaced doc comment.
  • Waiters of an invocation fenced inside a host call were not failed by the loop's stop.
  • A fence first seen by AppendInvocationIfVersion's commit was reported as a successful append.
  • Relinquished-agent removal was keyed only by id and could evict a newer generation.
  • Entity bodies were torn down as an API interrupt instead of ShardLost.
  • The owner could reuse a fork's unfenced target handle through the open-oplog cache.
  • The durable-stream producer published and indexed writes whose commit was fenced.
  • Durable-stream and p3 request-body paths flattened the fence into a string.
  • The new rejection reason was retried only on the unary path, not streaming.
  • A mixed-version rollout turned routing misses into protocol failures (documented).
  • rpc_error_from_rejection discarded the executor's rejection text.
  • The shard_epoch doc named readers that cannot see it; forks copied the source's epoch; old
    encodings without shard_epoch were never decoded by a test.
  • The e2e did not isolate what it claimed, could pass without any write being fenced, and probed
    liveness too early.
  • signal_child's safety claim did not hold after the child was reaped; the pause API failed
    misleadingly off unix; the 30 s lease override applied to every sharding test.
  • The new owner read the last index before claiming the epoch, so the loser could still append.
  • A revoke landing while a worker was built left an oplog with no epoch.
  • A refused begin marker still counted as committed, so the remote write ran.
  • oplog-commit(replicas) reported durability after a fenced commit.
  • Epoch regression blocked agents permanently.
  • A fenced or relinquished agent left no log line.
  • Migration 002 blocks rollback and a mid-rollout restart of an old executor (documented).
  • A fenced EndAtomicRegion and fenced p3 request-body frames were classified as generic failures.
  • Found after that review: a fork's unfenced archive transfer could delete the new owner's entries;
    ownership tests updated for give-up-on-revoke.

Final adversarial review — 38 candidates, 29 confirmed, from four root causes.

Refused commits were swallowed

  • A result was published after a refused AgentInvocationFinished commit.
  • A remote transaction's pre-commit marker was swallowed, so the database commit still ran.
  • A custom durable Start and three replay Jumps ignored a refusal.
  • Cancel, revert, update and plugin (de)activation were acknowledged after a refused write.
  • Durable streaming acceptance swallowed a refused commit.
  • A failed update on a fenced oplog recursed forever; foreign-mapping activation retried forever
    holding the instance lock.

The primary oplog stayed half-alive after the fence latched

  • A refused commit left an index gap, so the next exact read aborted the executor.
  • Panics and expects on invocation start/finish, HTTP responses, Worker::new and durable-session
    records now saw Fenced and aborted the executor.
  • The forwarding oplog's index went out of step after a refused add.

Relinquish mechanics

  • Relinquish deadlocked with the loop cancelling a completed invocation.
  • Relinquishing a loading agent stalled the renewal loop until the lease lapsed.
  • Two lost-shard exits never marked the agent given up; session completion missed Interrupted{ShardLost}.
  • Relinquished pending invocations were cached as a non-retriable failure.

Ownership races

  • get_or_open decided "I built this" from a shared flag; the dead-handle branch could remove a pending
    replacement. (The upstream merge replaced both with a per-agent lifecycle guard.)
  • Indeterminate-write reconciliation panicked instead of reporting the fence.
  • Aborting a transfer mid-step made the next transfer's identical chunk a key conflict.
  • A fork target's forwarding actor was aborted but never joined.
  • A wire epoch of u64::MAX overflowed in the shard manager.

Hygiene

  • Lib tests spawned processes; the walkthrough and durable-execution skill were not updated; the local
    debugging service read an empty Redis oplog; two tests could pass vacuously; an unchecked SQLite epoch
    cast; an interpolated warn!.

Refuted: 6 (pre-existing or already decided). Not addressed: epoch reissue after a shard-manager store
wipe (follow-up, below) and a push lost when a handler is cancelled mid-persist (unconfirmed).

Review of the fixes (three reviewers, then a re-review)

  • A manual update from inside the loop deadlocked with an outside stop (the same shape as the relinquish
    deadlock).
  • Foreign-mapping activation ignored a relinquish without a fence.
  • A lost shard without a fence still wrote FailedUpdate or a failed streaming session; replay toward an
    automatic update could too.
  • Two delayed-retry exits and a failed instance creation gave waiters a non-retriable answer; a stale
    handle could restart a given-up generation.
  • A 30 s activation budget gave up after the invocation was already accepted (budget removed).
  • The u64::MAX guard was off by one (u64::MAX - 1 still overflowed on the next reassignment).
  • The moved process-spawning tests were not run by CI.

Also fixed along the way

Found by the audits; each has a test.

  • Two writers at one epoch. A shard manager whose state was wiped mints from zero again and could
    hand a live owner's epoch to another executor; both passed the fence, because the record held only a
    number. The record now names the writing process as well, an equal epoch from anyone else is refused,
    and the refusal is reported so the manager mints past it. The writer is a per-process identity, not
    the executor's lease identity, which is regenerated on LeaseNotFound and would fence a process off
    its own agents after every re-registration.
  • Epoch overflow, end to end. A negative value in the epoch column read back through an unchecked
    cast as a near-u64::MAX epoch, was reported as evidence, and walked the manager's own mint to the
    ceiling, where it panicked. The reads are checked, the mint is fallible - a shard whose epoch cannot
    advance stays where it is instead of aborting the manager - and LeaseEpoch gained the checked
    advance its shard-side twin already had.
  • The WebSocket session API now reports the new rejection reason as its own retriable code rather
    than InternalError, so a session client reroutes like every other caller.
  • Quota leases: a RenewQuotaLease/ReleaseQuotaLease call with epoch u64::MAX panicked the
    shard manager. It is now an ordinary stale epoch.
  • Archive transfers: a byte-identical repeat of an already-stored compressed chunk is accepted
    instead of panicking on the key conflict.

Known gaps

Every open risk from the review was re-verified against this head. These are what is left.

Handed to #3766, which rewrites the revoke path this sits in:

  • suspend_worker still flushes the status blob for an agent relinquished by a revoke, and status
    writes carry no epoch, so a stale one can overwrite the new owner's.
  • drop_prefix is not epoch-checked, so a transfer that outlives its ownership can still trim or
    delete an oplog the new owner holds.
  • Revoke keeps this PR's relinquish sweep but skips upstream's owner-retirement steps: the producer
    fence, stopping the attachment reconciler, draining the lifecycle, and begin_delete on the status
    flusher and checkpointer. No durability impact - late writes are replayed or refused - but the
    liveness gaps are real.

Also for #3766: Worker::relinquish now returns without its own stop when a deletion already owns
the retirement, so the "relinquish awaits loop exit" contract no longer holds on that branch.

Accepted here:

  • The fence covers the oplog only. Key-value records, the status blob and blob payloads carry no
    epoch. The oplog - the only thing replay reads - stays correct; a stale status can be observed until
    the agent's next status change on its new owner.
  • Ephemeral oplogs are not fenced, by design: they are never replayed.
  • External effects. A call already in flight when the shard moves can happen twice, the same
    at-least-once exposure a crash has. A call that has not started is now refused before the effect
    runs, including for writes marked idempotent, which previously checked nothing until the commit
    after the effect.
  • Rollout is roll-forward only, and the shard manager and worker services go first. See
    deployment.
  • The shipped default is still KVStoreRedis, which cannot fence, so a distributed deployment on
    unmodified defaults fails at startup until the setting is changed. Changing the default is ticket 6.
  • The worker service's routing retry loop has no terminal case (pre-existing, untouched here): on
    exhaustion it computes a backoff, never awaits it, resets the counter and loops. This PR routes a new
    and more frequent failure into it, so a fence that does not resolve presents as a request that never
    finishes. Worth fixing deliberately rather than inside a fence PR.

Resolved with evidence, not left open:

  • A push lost when a handler is cancelled mid-persist (flagged uncertain in review): the durable-stream
    write runs on a spawned actor task and is queued before the caller's only await point, and the effects
    guard poisons the producer rather than reporting success. Not a defect.
  • A stale trailing compressed chunk: pre-existing upstream behaviour - abort_transfer_in_drop is
    older than this branch - and the orphan is swept by the next drop_prefix.

How it is tested

Suite Result
Executor lib 2217 passed
Executor integration: indexed_storage (5 backends, PostgreSQL in Docker) 194 passed
Executor integration: active_agents, hot_update, worker_initialization, ownership/fence/relinquish tests in api 53 passed
Shard manager lib / integration (incl. etcd) 120 / 134 passed
Worker service lib 359 passed
golem-common lib 1268 passed
Sharding e2e (cargo make sharding-tests-debug) 10 / 10 passed
check-openapi (spec), check-configs, check-diff-model-fingerprint, check-wit clean
  • Storage: the compare-and-set, the fenced append on stale, ahead and missing records, delete,
    None bypassing the check, and that a refused batch leaves nothing behind, on every backend
    (non-fencing backends must accept the same writes). Deliberately breaking each backend's check turns
    the suite red.
  • Oplog service over a SQLite store that fences: an owning epoch writes; a stale epoch is refused at
    its first write and stays refused; once latched, adds below the commit threshold are refused and
    earlier indices stay readable; a writer holding a deleted oplog is refused; an indeterminate write on
    a moved shard is reported as fenced; a fenced cached handle is never handed out again.
  • Executor: a fenced write escaping a host call classifies as ShardLost and a transient storage
    failure does not; every lost-shard exit path gives the agent up; a slow sweep does not stall renewal
    and concurrent announcements coalesce; an invocation enqueued onto a fenced oplog is refused before
    acceptance with SHARDING_NOT_READY; a caller waiting on an invocation fenced inside a host call is
    told to reroute, and the executor writes nothing after it.
  • The writer column: on every backend, a second process presenting an epoch the record already
    holds is refused and the refusal names the writer as the reason; the same process re-opening at the
    same epoch is accepted, which is what happens on every cache eviction; and a newcomer minted above
    the collision takes the oplog over, after which the old owner is the one refused. On the manager
    side, a fence reported by a shard's own assignee at its own epoch is minted past, while the same
    report from anyone else is the ordinary loser of a shard move and moves nothing.
  • Loop-side guarantees that previously had no deterministic test: a stop racing a manual update
    finishes both ways round rather than deadlocking; a retry scheduled before a revoke does not resume
    an agent this executor gave up; a start through a relinquished generation is refused and leaves the
    generation that replaced it alone; a foreign stream mapping on a fenced oplog reports the fence,
    commits nothing and releases the session lock.
  • End to end
    (an_executor_paused_until_its_shards_move_cannot_finish_the_invocations_it_started): eight agents
    each start an in-flight invocation; three of four executors are frozen until their shards move; the
    fourth takes the shards over and finishes the work; the frozen ones are thawed. Each invocation
    returns exactly once, each agent gains exactly one AgentInvocationFinished for the method, the
    stored owning epoch has moved, and every executor still serves afterwards. With the PostgreSQL append
    check disabled the test fails: a thawed executor's stale append collides with the owner's and aborts it.

🤖 Generated with Claude Code

@netlify

netlify Bot commented Sep 9, 2026

Copy link
Copy Markdown

Deploy Preview for golemcloud canceled.

Name Link
🔨 Latest commit 0b01ed2
🔍 Latest deploy log https://app.netlify.com/projects/golemcloud/deploys/6aad15832600520008f4b11b

@Aditya1404Sal
Aditya1404Sal force-pushed the shard-manager/ticket5-oplog-epoch-fence branch 4 times, most recently from da46c95 to 04f87cc Compare September 11, 2026 12:12
@Aditya1404Sal
Aditya1404Sal force-pushed the shard-manager/ticket5-oplog-epoch-fence branch from f4830c0 to d29fdfd Compare September 14, 2026 19:43
@Aditya1404Sal
Aditya1404Sal marked this pull request as ready for review September 18, 2026 11:30
@Aditya1404Sal
Aditya1404Sal requested a review from a team September 18, 2026 11:30
// The request reached an executor that does not own the agent's shard: a stale route, or an
// executor whose lease has lapsed. Not a refusal of the invocation - the caller retries it on
// the shard's owner after refreshing its routing table.
INVOCATION_REJECTION_REASON_SHARDING_NOT_READY = 15;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already had an InvalidShardId case of WorkerExecutorError that was explicitly handled in the worker service layer by retrying with an updated routing table etc. I don't see how this fits into that?

},
/// The agent has been invoked
#[desert(evolution(FieldAdded("wallet_pin", None::<InvocationWalletPin>)))]
#[desert(evolution(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add evolution steps until we start keeping backward compatibility. They are not free (increasing the serialization size and decreating deserialization performance)

trace_states: Vec<String>,
invocation_context: Vec<SpanData>,
wallet_pin: Option<InvocationWalletPin>,
/// The shard epoch this executor held for the agent's shard when the invocation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'converting back yield None' part sounds problematic
Also we don't keep backward compatibility so there are no "entries written befor ethe fence existed"

/// moving, or a write of its was fenced by the shard's new owner. Nothing is wrong with the
/// request, and a client that reconnects reaches the new owner - which is why this is not
/// `InternalError`.
ShardingNotReady,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see my previous comment about this error - but in case we need this new error, I still don't agree with its name :) 'shard is owned by another executor' does not mean 'sharding is not ready' - or maybe I misunderstand something?

);
}

/// Freezes this worker executor's process in place (SIGSTOP) without killing it: it keeps its

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this helping with testing real-world scenarios? We already had sharding tests killing executor processes. Isn't that a better representation of what happens in production? (Loosing an executor node)

Is this for simulating other kind of failure scenarios?

test_r::enable!();

#[cfg(unix)]
mod unix {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this and what (not) happens on windows?

///
/// The default does nothing and accepts everything: a backend that cannot fence has no record
/// to keep.
async fn upsert_oplog_metadata(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indexed storage abstraction does not know about oplogs and we should not introduce that concept on this level. This makes it simpler to implement alternative storage backends.

That's why namespace is an abstract concept on this level and there is no logic in the indexed (and other) storage providers that depend on the actual use case.

);
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit nervous about the amount of code changed in the oplog layers in general - I understand the introduction of reporting back fenced error instead of crashing the executor. But for all the other changes it is hard to me to see whether it's all necessary and keeping all the edge cases working as intended. Hopefully we have tests covering them - but I'd take another round to try to minimize the executor-level changes

}
}

/// Appends one already-serialized compressed chunk, retrying transient failures like

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these changes?

/// No: this executor owns the single shard for its whole life and nothing can take it away,
/// so there is no second writer to fence out.
fn requires_oplog_fencing(&self) -> bool {
false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this distinction? Can we make things simpler by not branching on tihs?

use crate::worker::{
CreateWorkerInstanceError, FinalWorkerState, PendingLiveInvocationDisposition,
PendingWorkerInterrupt, QueuedWorkerInvocation, RetryDecision, RunningAgent,
PendingWorkerInterrupt, QueuedWorkerInvocation, RelinquishReason, RetryDecision, RunningAgent,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I maybe wrong here (and the comment is about the whole invocation loop / Worker lifecycle part) but I'd expect this to be simpler with less changes. Why is the fencing error different than a regular non-retriable trap?
Sure there are some differences (as in not writing an Error oplog for example) but the worker lifecycle already supports this kind of stopping of the agent.

cache_retirement_in_progress: AtomicBool,
/// Set once this executor has given the agent up. One-shot: the first reason wins, and the
/// agent is never revived here.
relinquishment: std::sync::OnceLock<RelinquishReason>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use this word :) (anywhere) - it's not a common word, I had to look it up in a dictionary, this make the code hard to understand for non-english speakers.

ArchiveWait, CommitLevel, EphemeralOplog, MultiLayerOplog, Oplog, OplogLifecycleGuard,
OplogOps, downcast_oplog,
ArchiveWait, CommitLevel, EphemeralOplog, MultiLayerOplog, Oplog, OplogError, OplogFence,
OplogLifecycleGuard, OplogOps, downcast_oplog,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same general comment that I wrote to the invocation loop also applies to the changes in this file - it seems too much, touching everything - I cannot point to a particular set of lines that are unnecessary but I just feel this should not be like this

warn!(%error, "Committing the oplog while stopping failed");
false
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Classify the final commit’s fence before publishing pending-invocation failures.

The running-worker stop branch calls fail_pending_invocations before this final oplog commit. If this commit is the first operation to discover lost ownership, the stop has already cached and published an ordinary terminal failure rather than a routing error.

For example: executor A begins deletion with an accepted invocation and a buffered oplog entry; executor B claims the oplog at a higher epoch before A’s invocation loop processes the interrupt. A publishes “Worker is being deleted” to its invocation waiters, then its final commit is fenced and marks the worker relinquished. The later relinquishment notification cannot repair this: fail_pending_invocations skips already-valid cached results before reaching its relinquished branch. The new owner can still execute the invocation, while its caller has received a terminal failure.

Please perform the final commit and classify/mark any fence before failing pending invocations, while still completing the stop after refusal. A regression test should keep the guest from processing its interrupt, buffer an entry below the commit threshold, advance the stored epoch through a second writer, and drive an external stop carrying a non-routing error. Assert that callers receive a routing error and no ordinary invocation failure is cached. The buffered entry is important because an empty commit does not query storage.

Found by Amp’s oracle review; statically traced, not locally reproduced.

.await
{
Ok(assignment) => {
self.carried_claim.write().unwrap().clear();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Automatic epoch recovery also needs to recover delivery ordering.

A surviving executor retains its last-applied delivery revision when clear_assignment() removes its shards. After a shard-manager store wipe, this re-registration can correctly repair shard epochs but return a revision below the executor’s retained revision. ShardService::register passes the grant through ShardAssignment::apply, which rejects it as stale; the outcome is ignored here and the carried claim is cleared.

For example, an executor that applied revision 10,000 rejects a repaired registration at revision 3 and remains on an empty assignment. Subsequent deliveries are also rejected until the replacement manager’s revision catches up or the executor restarts. Restoring a backup that still recognizes the executor has the analogous problem on the renewal path. Renewals do advance the manager revision, so this is not necessarily permanent, but recovery is not bounded by the one renewal interval promised in the new deployment documentation.

The revision gate predates this PR; this is an incomplete integration of the newly added automatic history-repair feature, rather than a new revision-ordering regression. Please add tests with a high previously applied revision and a low repaired grant for both wiped-store re-registration and restored-store renewal. The current registration test helper always returns revision 1 and misses this case.

Repair needs to preserve delivery ordering across the reset—for example, by carrying last-applied revision evidence and minting repair deliveries above it, or by introducing a manager-incarnation ordering domain. Simply resetting the local revision must not allow delayed old-manager deliveries to take effect.

Found by Amp’s review and confirmed by the oracle; statically traced, not locally reproduced.

@vigoo vigoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings from an Amp-assisted review of the head commit (0b01ed2). Verified by reading the diff and running: cargo check --tests for golem-worker-executor, golem-shard-manager and golem-worker-service (clean); cargo test -p golem-shard-manager --lib (120 passed); cargo test -p golem-worker-executor --test integration -- indexed_storage:: (194 passed across all five backends, Postgres via Docker). The e2e sharding tests were not run.

Two items that have no diff line to anchor to:

  • golem-worker-service/src/service/worker/routing_logic.rs ~L544: the retry-exhaustion branch computes delay and logs delay_ms but never sleeps. Pre-existing on main, not introduced here — mentioning it because the PR body calls it out and it is trivially fixable while you are in the file.
  • Comment style overall: many comments narrate review history and previous shapes of the code ("the old set_interrupting(..) shape risked", "F29", commit hashes). AGENTS.md asks for comments that describe the code as it is; please trim them in the next round.

Checked and found correct, for the record: Postgres SELECT … FOR UPDATE CAS and the monotonic upsert_oplog_metadata; whole-batch rollback on SQLite/multi-SQLite; fence propagation through the multilayer/compressed/plugin layers; fork target opening unfenced; fenced AgentInvocationFinished not being published; idempotency-key dedupe across a retry to the new owner; OplogError::Storage still panics rather than being swallowed.

indexed_storage: &(dyn IndexedStorage + Send + Sync),
indexed_storage_config: &IndexedStorageConfig,
) -> anyhow::Result<()> {
if requires_oplog_fencing && !indexed_storage.supports_epoch_fencing() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — The shipped default config cannot boot a distributed executor after this lands.

The standalone executor always uses GrpcShardManagerService (lib.rs:214), whose requires_oplog_fencing() is true, and golem-worker-executor/config/worker-executor.toml (plus the sample env) still defaults indexed_storage.type to KVStoreRedis, which cannot fence. So an unmodified default deployment fails here at startup. The PR body defers the default change to "ticket 6", but merging leaves main with a non-booting default; CI won't notice because the test framework and docker-examples already use Postgres/SQLite.

Please either change the shipped default in this PR or downgrade this guard to a warning until the default moves.

@@ -2446,6 +2449,9 @@ fn safe_rejection_message(code: PublicErrorCode) -> String {
PublicErrorCode::ProducerError => "stream producer failed",
PublicErrorCode::InvocationFailed => "invocation failed",
PublicErrorCode::ProtocolError => "invocation protocol failed",
PublicErrorCode::ShardingNotReady => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — sharding-not-ready is sent with retryable: false.

The rejection frame builder (around line 1793 in this file) uses retryable: matches!(code, PublicErrorCode::ResourceExhausted), so this new code ships retryable: false while the message here tells the client to retry. A client honouring the flag gives up on a transient condition. Please include ShardingNotReady in the retryable set (and add it to the error-code table in docs/src/content/next/invoke/stream-session-public-protocol-v1.mdx, which does not list it).

_ => match self.shard_epochs.get(&shard_id) {
Some(last) => last.next(),
None => ShardEpoch::initial(),
Some(last) => last.checked_next().filter(|epoch| epoch.0 != u64::MAX),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Ceiling handling is inconsistent between floor-raise and mint.

raise_epoch_floor_for accepts and stores u64::MAX - 1 (the test at ~1523-1535 asserts this), but this mint refuses to produce u64::MAX, so a shard sitting at MAX - 1 can never change owner again. Either refuse MAX - 1 in the floor raise too, or let the mint return MAX and refuse only the following one. Practically unreachable for honest executors, but see the trust comment on the floor raise: a client can put a shard there deliberately.

continue;
};
self.shard_epochs.insert(*shard_id, epoch);
if let Some(entry) = self.shard_assignments.get_mut(shard_id) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Client-supplied epochs are trusted verbatim (design question).

previous_shard_epochs on register and fenced_shard_epochs on renew are applied here without checking that the caller owns the shard. Any executor can therefore raise the floor on a shard owned by someone else (this branch rewrites entry.epoch on the live assignment, forcing the real owner to relinquish it on the next renewal) or pin a shard at u64::MAX - 1. If executors are trusted peers this is acceptable, but please state that assumption in the shard-manager docs; a cheap hardening is to accept a floor raise only for shards the caller owns or that are unassigned.

/// machine has an arm for each, so none is skipped. An agent still being resolved is not in
/// it, as a creation in progress never was: see `shard_epoch_to_assert` for why its oplog
/// cannot open unfenced on a shard that has already left the assignment.
pub(crate) async fn relinquish_matching(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — The sweep misses agents that are still resolving, and this comment overstates the guard.

snapshot() only yields resolved_primary(). An agent that read the assignment before the revoke and then opened its oplog at the old epoch is never selected here, so it stays cached and unfenced until the new owner claims the metadata row. shard_epoch_to_assert does not cover it because the assignment it read still contained the shard. Durability is fine (the new owner's claim precedes its read of the last index, so late writes are fenced), but the agent lingers on this executor for the whole window. Consider re-running the sweep once resolution completes, or documenting this as a bounded liveness window instead of claiming it cannot happen.

// write pool is capped at a single connection (golem-service-base
// db/sqlite.rs:46-50), so this transaction holds the only writer and the
// check cannot be interleaved. Raising that cap means switching this to
// `BEGIN IMMEDIATE`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — the single-writer claim holds per process only.

The one-connection write pool serialises writers inside this executor. Two executors sharing one SQLite file would rely on SQLite's lock upgrade from the deferred begin() returning SQLITE_BUSY — safe, but that surfaces as a storage error, not a fence, and the comment does not say so. Either state that shared-file SQLite is unsupported for multi-executor deployments, or use BEGIN IMMEDIATE and say why.

make_batch: DurableStreamBatchBuilder,
) -> Result<Vec<(OplogIndex, OplogEntry)>, String> {
) -> Result<Vec<(OplogIndex, OplogEntry)>, OplogError> {
let result = self.inner.add_durable_stream_batch(make_batch).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — the rate limit is awaited before returning an already-fenced error.

If inner.add_durable_stream_batch returned Err(OplogError::Fenced), there is nothing to rate-limit; return early on Err so a relinquishing worker does not sit in the limiter.

// unfenced handle this service still holds at the same epoch is handed back without
// asking the storage, and then this is the only refusal before a write.
let fenced_at_create = match shard_epoch {
Some(epoch) => record_owning_epoch(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — create/create_fresh upsert the owning epoch twice.

This call records the epoch, and open_with below calls record_owning_epoch again in the PrimaryOplog constructor (~line 1143). Two round trips per create. More generally every open now costs one metadata write; consider a metric for oplog_metadata upserts/refusals so the cost is visible in production.

ProducerError,
InvocationFailed,
/// The executor that answered does not own this agent's shard right now: the assignment is
/// moving, or a write of its was fenced by the shard's new owner. Nothing is wrong with the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — typo: "a write of its was fenced" → "one of its writes was fenced".

// reaped on a 20s tick, an unrenewed lease is gone within 80s of the pause. That bound
// keeps a thawed executor from waking up as the owner; it does not stretch the callers'
// retries, which count on the health check.
tokio::time::sleep(Duration::from_secs(90)).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — this test does not deterministically prove a fenced write.

It relies on fixed sleeps (2 s here, 90 s below) and accepts either relinquish-by-assignment or a storage fence, asserting only that some epoch increased. The storage-level tests in golem-worker-executor/tests/indexed_storage.rs cover the fence itself, so this is a coverage note rather than a blocker, but a variant that holds the old owner frozen past the lease and then asserts a specific Fenced refusal on it would make the e2e meaningful.

@vigoo

vigoo commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

I asked another agent to review my own comments above (not the agent-provided ones), pasting here the output, hoping it helps.

InvalidShardId vs ShardingNotReady (proto:244). WorkerExecutorError::ShardingNotReady already exists on main; what's new is the streaming InvocationRejectionReason. On main the invocation-session path maps InvalidShardId to Internal (_ => Internal), so a session client could not tell a routing miss from a failure — that gap is real. But the PR collapses three things (lines 2696-2698): InvalidShardId, ShardingNotReady, and OplogFenced. The unary path keeps InvalidShardId { shard_id, shard_ids } and uses the shard_ids payload to fix the routing table locally without a round trip (routing_logic.rs:281-300); the streaming rejection throws that payload away. Suggestion for the implementor: make the streaming rejection carry the existing WorkerExecutionError (so InvalidShardId keeps shard_ids) rather than adding a new reason; and if a new reason is kept, name it for what it is (ShardNotOwned / RoutingMiss). Also worth asking: for OplogFenced mid-invocation, is retrying on the new owner always safe? It relies on idempotency-key dedupe, which is fine for agent invocations but should be stated.

Evolution step and shard_epoch on AgentInvocationStarted (mod.rs:188, 203). Stronger version of your point: the field is write-only. It is set in oplog/mod.rs:1101 and every reader either ignores it (public oplog, WIT) or clears it (worker_fork.rs:832). Nothing in the fence logic depends on it — the fence lives in oplog_metadata. Deleting the field removes the evolution step, the fixture test, the protobuf changes, the fork-clearing code, and my WIT nit in one go. I'd tell the implementor to drop it unless there is a concrete reader planned.

Storage abstraction knowing about oplogs (indexed/mod.rs:447). Agree. The atomic epoch check has to live in the storage transaction, so it cannot move entirely up a layer, but it can be made generic: a per-key writer generation (set_key_epoch/append(.., expected_epoch)), table indexed_key_epoch or similar, no mention of oplog/agent/shard. One more thing worth flagging to the implementor: the trait's default upsert_oplog_metadata is a silent no-op Ok(()) (mod.rs:447-455). A new backend that forgets to implement it would appear to work and fence nothing; that's why the PR needed the supports_epoch_fencing() flag plus the startup guard. Making the methods required (no default) removes the need for both.

Amount of executor change (primary.rs:533, invocation_loop.rs:36, worker/mod.rs:83). I share the unease; here is what I think is separable so the implementor can cut rather than defend everything at once:

  • Necessary core: append takes an epoch and returns Fenced; Oplog::commit (and add, because add can trigger a threshold commit) propagate OplogError; the worker turns it into TrapType::Interrupt(ShardLost) and stops without writing an Error entry. That is a few hundred lines, not 4,900.
  • Questionable extras: the OnceLock fence latch keeping refused entries "readable" at the head of the buffer; the relinquishment: OnceLock<RelinquishReason> marker on Worker with reason variants; the OpenOplogs epoch-based cache eviction; the compressed.rs:81 duplicate-chunk reconciliation. The last exists only because the transfer to lower layers is not epoch-protected, so two executors can both archive. Fencing the archive append with the same epoch (only the owner archives) would make that reconciliation unnecessary and is simpler than tolerating duplicates.
  • Your question "why is it different from a regular non-retriable trap" is the right lever: once the worker is stopped by the interrupt and dropped from ActiveAgents, the pending invocations fail with a routing error that worker-service already retries. Most of the lifecycle changes seem to exist to make the old owner behave nicely after losing the shard, which the fence already makes unnecessary.

SIGSTOP/SIGCONT pause (test-framework:56). Here I'd partly disagree with your comment, and it may be worth softening for the implementor: killing an executor cannot produce the failure the fence guards against. The fence exists for a live-but-stalled executor (long GC pause, network partition, slow disk) that resumes with a lapsed lease and keeps writing. SIGSTOP/SIGCONT is the standard way to simulate exactly that. So the pause is the right tool — the problem is the test using it (sharding.rs:363-384) does not assert that a fenced write happened on the resumed executor; it only checks that some epoch increased. Ask for that assertion rather than for removing the pause. On Windows the helper is #[cfg(unix)]-gated and the tests are simply absent, which is acceptable but should be said explicitly.

signal_unreaped_child test (line 23). Adds to your "why": it also violates the AGENTS.md rule that only CLI integration tests spawn processes; my comment on Makefile.toml:728 says so, so the two reinforce each other.

requires_oplog_fencing distinction (shard_manager.rs:824). Agree it should go. It exists only to make the single-binary/debugging-service path skip the startup guard. If the storage methods become required (above) and the shipped default is changed to a fencing backend (my P1), the guard and the flag can both be deleted.

"relinquish" (worker/mod.rs:641). Agree; give up/release/lost read fine. It appears in type names (RelinquishReason, relinquish_matching, Worker::relinquish), so it's a rename across ~5 files, not just the comment.

Your two > P2s (mod.rs:8209 ordering of fail_pending_invocations vs final commit; shard_manager.rs:690 revision gate after store wipe). Both read as plausible from the code paths cited; I did not independently trace or reproduce either, so I have nothing to add or contradict there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants